Add trustpub_only flag
#12365
Add trustpub_only flag
#12365
Conversation
Turbo87
added
the
C-enhancement ✨
Turbo87
force-pushed
the
trustpub-only
branch
2 times, most recently
from
4c4deb5 to
1b5fb23
Compare
LawnGnome
left a comment
LawnGnome
left a comment
There was a problem hiding this comment.
LGTM. One or two comments, but the implementation looks good!
| && matches!(auth, AuthType::Regular(_)) | ||
| { | ||
| return Err(forbidden( | ||
| "You tried to publish with an API token but this crate requires trusted publishing.", |
There was a problem hiding this comment.
I think people may be confused by "API token" as a term of art, so how about:
| "You tried to publish with an API token but this crate requires trusted publishing.", | |
| "New versions of this crate can only be published using Trusted Publishing.", |
There was a problem hiding this comment.
hmm... I specifically wanted to include the "API token" term to make it more obvious to the user what the problem is. I'm not sure what exactly you mean by "term of art". Do you think that crate owners that try to publish something are not aware of what the term "API token" means? 🤔
There was a problem hiding this comment.
Do you think that crate owners that try to publish something are not aware of what the term "API token" means?
Honestly, yes, especially in a scenario where this might be an error message they get some time after they configured their crate to require TP. I could see a crate owner — particularly one that's stressed because they're trying to publish a fix for a vulnerability or critical bug — throwing their hands up and wondering how a TP token differs from an API token.
I don't mind including it somewhere in there in a second sentence, but I think we need to frontload the actionable part, which is that Trusted Publishing is required.
| {%- endif %} | ||
|
|
||
| {% if trustpub_only -%} | ||
| This crate can now ONLY be published via Trusted Publishing. Publishing with API tokens has been disabled. |
There was a problem hiding this comment.
(to follow on from my previous comment, I don't have the same concern over the use of "API token" here because the user is likely to have immediate context, since they or another owner were just thinking about trusted publishing)
Turbo87
force-pushed
the
trustpub-only
branch
from
1b5fb23 to
e93afe2
Compare
eth3lbert
left a comment
eth3lbert
left a comment
There was a problem hiding this comment.
Left a minor nit. Overall LGTM! thanks!
| // Update trustpub_only if provided | ||
| if let Some(trustpub_only) = body.trustpub_only { | ||
| diesel::update(crates::table) | ||
| .filter(crates::id.eq(krate.id)) | ||
| .set(crates::trustpub_only.eq(trustpub_only)) | ||
| .execute(conn) | ||
| .await?; | ||
|
|
||
| // Audit log the setting change | ||
| info!( | ||
| target: "audit", | ||
| action = "trustpub_only_change", | ||
| krate.name = %krate.name, | ||
| network.client.ip = %**real_ip, | ||
| usr.id = user.id, | ||
| usr.name = %user.gh_login, | ||
| "User {} set trustpub_only={trustpub_only} for crate {}", | ||
| user.gh_login, | ||
| krate.name | ||
| ); | ||
|
|
||
| // Send email notifications to all crate owners | ||
| for (_, gh_login, email_address, email_verified) in &user_owners { | ||
| if *email_verified { | ||
| let email = TrustpubOnlyChangedEmail { | ||
| recipient: gh_login, | ||
| auth_user: user, | ||
| krate, | ||
| trustpub_only, | ||
| }; | ||
|
|
||
| if let Err(err) = email.send(app, email_address).await { | ||
| warn!("Failed to send trustpub_only notification to {email_address}: {err}"); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not a blocker, but I'm wondering if we should check whether the given value is different from the existing value since we are sending a notification?
Nevertheless, I especially love the introduced notification mechanism here. Without it, a malicious user with a leaked API token could change the setting and publish a malicious version, which the crate owner might not notice. But with this mechanism, the crate owner has a chance to rotate the token and make amends!
4 participants
This PR implements the backend part of #12361.
It adds a new
trustpub_onlycolumn to thecratestable, exposes the field in the API responses, adjusts the publish endpoint to check it, and implements a newPATCH /api/v1/crates/{name}endpoint to toggle it.The UI implementation will follow in a dedicated PR.